Use special DefIds for aliases#155981
Conversation
| Err(TypeError::ProjectionMismatched(ExpectedFound::new( | ||
| a.def_id.into(), | ||
| b.def_id.into(), | ||
| ))) |
There was a problem hiding this comment.
can you update TypeError instead?
There was a problem hiding this comment.
I can't because unlike the name the def id can be any alias; see <AliasTy as Relate>::relate().
There was a problem hiding this comment.
ah, we also need to update the Relate<I> for AliasTy impl 👍 #154758
81000a1 to
5e8cb76
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use special DefIds for aliases
|
❗ There is currently no auto build in progress on this PR. Hint: There is a pending try build on this PR. Maybe you meant to cancel it? You can do that using |
|
@bors try cancel |
|
Try build cancelled. Cancelled workflows: |
|
How do I request a perf run with |
|
Seems like there is no way to do that, so at least... @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use special DefIds for aliases
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (a2fdd5d): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -5.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 494.198s -> 495.332s (0.23%) |
5e8cb76 to
da673e8
Compare
|
Converted to draft because alongside this PR I'm also porting r-a to it, so I want to make sure I don't miss anything. |
fa67dc2 to
598bf01
Compare
|
Okay, I've verified that the changes here work for r-a. |
| pub(super) fn normalize_anon_const( | ||
| &mut self, | ||
| goal: Goal<I, ty::NormalizesTo<I>>, | ||
| def_id: I::UnevaluatedConstId, |
There was a problem hiding this comment.
hmm, passing the DefId in separately feels redundant 🤔
could you do
pub struct NormalizesTo<I, K = AliasTermKind<I>> {
pub alias: AliasTerm<I, K>,
pub term: Term<'tcx>,
}oh actually, that's just a very big change xd
we should have a single
struct Alias<I: Interner, K> {
kind: K,
args: I::GenericArgs,
}this is not something we should do in this PR but as a separate cleanup. so this is fine for now
Like we do for other things for better experience in rust-analyzer. It's possible now that the `AliasTyKind` and `AliasTermKind` contains the DefId. It does require a few `try_into().unwrap()`s since in the solver's `consider_X_candidate()` only get an untyped `DefId`. It's possible to reduce that considerably if we'd pass them the typed def id as a parameter, but I don't know what will be the impact on perf.
598bf01 to
54685c8
Compare
|
@lcnr Changed according to comments. |
View all comments
Renewal of #155025, after
AliasTermKindwas also ported.Like we do for other things for better experience in rust-analyzer.
It's possible now that the
AliasTyKindandAliasTermKindcontains the DefId.It does require a few
try_into().unwrap()s since in the solver'sconsider_X_candidate()only get an untypedDefId. It's possible to reduce that considerably if we'd pass them the typed def id as a parameter, but I don't know what will be the impact on perf. Should I try to pursue that?r? lcnr